-
Notifications
You must be signed in to change notification settings - Fork 521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #4652 : Implemented Learner Progress for Exploration State #5388
Conversation
@BenHenning, @adhiamboperes Would this approach be effective? |
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
@adhiamboperes PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rd4dev! This approach is really intersting. I'm handing over to @BenHenning to verify the approach, and @seanlip to look over the progressbar design for Product.
As a workaround to the Espresso run issues, I usually modify the test setup to unregister IdlingResource immediately after the profiles have been setup in the setUpTest()
Function. This should allow you to write tests.
THis is quite interesting, thanks @Rd4dev! From the product side, I don't have an objection, it would actually not be a bad idea to test this. The only request I have is to put this functionality behind a feature flag so that we can do some controlled testing before launching it more broadly. The only other concern I have is that it's a bit weird how the content gets "sucked up" into the progress bar -- I'm not sure about the handling of the cutoff when scrolling. Would you mind filing an issue on the design team repo to take a look at this, and I will talk to the Android design team to see if they can look at it? This doesn't need to block the merge of this PR though (assuming that the functionality remains gated behind a feature flag). Thanks! |
Thank you for the initial approval, @seanlip, I've filed a new issue in the design team repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rd4dev! I moved this over to draft because I think it has a lot of iteration needed before it's actually ready to be finalized and go into full review. I left one main comment to analyze the algorithm first--once we finalize that, I'll have more comments on how to model the dataflow and API changes. After that's done, the final steps are focusing on the UI and testing side of things.
This PR will probably require a lot of back-and-forths to finalize given the nature of this problem: modeling progress is complex in Oppia lessons since they have non-linear paths that are potentially unique to an individual's learning experience. We've fortunately thought about this in some detail, but if you're unsure whether you'll have the time to finish this PR let me know and we can move the discussion for the algorithm over to the issue for whoever works on it next.
@@ -68,4 +73,41 @@ class ExplorationRetrieverImpl @Inject constructor( | |||
} | |||
return statesMap | |||
} | |||
|
|||
private fun parseJsonObjectToCalculatePosition(explorationObject: JSONObject): Pair<Int, MutableMap<String, Int>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanlip has anyone worked on the in-lesson progress bar on web? I'm wondering if anyone has already come up with an algorithm for computing the linear path through the lesson that we could copy here for parity.
@Rd4dev one potential issue I see with this approach is that it's sort of implicitly assuming that each destination is an equal outcome when making progress. So long as we never let the user go backwards in progress, that helps a bit with making sure the progress bar looks correct, but it may be a bit unpredictable since sometimes states with many outcomes will show "more progress" of completing than those with few.
I think the intended way to implement this is to use a pathfinding algorithm from the starting state to all ending states. There should be only one path (but we'll need some sort of tiebreaker if there is more than one). From there, we only count progress for states who have checkpoints marked (this is a property on Oppia web we don't currently import: https://github.com/oppia/oppia/blob/5ebbb3d369532a22ee1638a810d597ffb9719594/core/domain/state_domain.py#L3615).
@seanlip does the algorithm explained in the above paragraph sound correct to you (in the event there isn't an existing implementation to follow)? Also, is the checkpoint field being set in lessons now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenHenning Yes, we have done a bit of work here. We label some states as checkpoints, and use those checkpoints in the lessons as a measure of progress. The checkpoint states are always located at articulation points of the graph (i.e. if you remove the checkpoint state then it disconnects the graph). Lessons have 2-8 checkpoints and progress is measured by the achievement of checkpoints. More granular progress is not recorded.
The checkpoint field is being set in lessons, and the third paragraph you mention seems like a good approach to follow. It does match what we do on Web. You don't need a tiebreaker, given the "articulation points" property above.
Please let me know if you need more info. Also, I apologize for entirely missing this in my previous comment -- for some reason I didn't make the connection between the progress bar and the existing checkpoints functionality on Web.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach sounds good and we should be able to utilize the new field.
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
@Rd4dev are you planning on working on this? Otherwise, I think it might be best if we close it and unassign the issue so that others can take it up if they have availability. It sounds based on Sean's latest feedback that we'd need to update the import asset script & domain protos for lessons to pull in the checkpoint properties so that we have a baseline for computing progress through an exploration. |
@BenHenning I don't think I will be able to work on this right now. I was initially looking into pathfinding algorithm implementations, but after Sean mentioned the necessary asset updates, I think it's best to unassign myself for now. I'll let someone else take over, and if it's still available later, I'll pick it up when I can. For now, I'll unassign myself. Thanks for the ping. |
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Fixes #4652
Explanation
TODO:
Note:
Essential Checklist
Demonstration
StateProgress3.mp4